Conversation
nrichers
left a comment
There was a problem hiding this comment.
Generally, this PR does exactly what it says on the tin when testing and I was able to test the core functionality changes. I also tried removing a copyright header from an unrelated Python API file and the make actions continue to work as expected.
That said, I have two comments:
- I had a "huh, this looks like VS Code?" moment when I tested: e2e_template.py is defaulting to VS Code when we should use Cursor AI at this point. Please either default to Cursor (
s/code/cursor/)or make this a configurable choice. - The copyright notice for notebooks continues to be unnecessarily wordy, only because it tries to mimic what our plain text copyright headers do. All you need is a simple link to the LICENSE file, let the hyperlink to the heavy lifting for where to find the notice. I made a suggestion to this effect.
Related, if I may: please start exploring what Cursor has to offer, even if you don't yet use much of it ... It initially feels very much like VS Code but you'll quickly discover that it has agent features that are much better. We pay for a team license for us to use it. And, I can see a day not too far in the future where working on docs PRs will require you to use Cursor (or de facto already requires it if you want to do follow the current release notes process I use ...).
Co-authored-by: Nik Richers <nik@validmind.ai>
13ab799 to
72488a2
Compare
@nrichers I don't actually know how to change this or what you're referring to, so you might need to help here. EDIT: What I mean by that is your link just takes me to... the full file, if you meant to highlight a line of something. Also, this script was created months, maybe over a year ago, when we were still all on VSCode, so that might have something to do with it? If you want significant updates here you'll need to provide some more guidance/suggest the changes yourself as I am not quite clear just based on the comment what changes you want to see. |
|
Re: force-push, sort of an accident — Not sure what happened in my logic and I think I was confused about the notebooks already being updated somehow and got lost. I don't even remember what I was trying to undo! |
…pt' of github.com:validmind/validmind-library into beck/sc-13985/update-notebook-templates-templating-script
|
|
nrichers
left a comment
There was a problem hiding this comment.
LGTM! 🚀 Thank you for making the switch to the simplified license footer. As discussed, I pushed a commit to call Cursor instead of VS Code and retested.
Ah, I see what you were referring to now, I didn't understand what you meant by default until now. I would prefer if we aligned with the option the user was currently using, instead of forcing something they might not have open. I'll take a look at the options. |
Yes, that would be much more elegant. What your describing is exactly what happened to me: I was testing your PR on Cursor when all of a sudden VS Code popped up. |
It should now "elegantly" open in the editor you're actively in, in theory beyond VS Code or Cursor (and in multiple OS), or in your system default editor, courtesy of Cursor's suggestions. |
PR SummaryThis pull request introduces extensive enhancements to how ValidMind copyrights are handled across the repository. In particular, it:
Overall, this PR standardizes the copyright header across the repository and solidifies its enforcement using helper scripts, contributing to improved compliance and consistency across documentation. Test Suggestions
|
Pull Request Description
What and why?
As a follow-up to adding a copyright to our notebook files, the notebook templating script now appends copyright information, and two new scripts that check for missing copyright info (
scripts/verify_notebook_copyright.py) and that add/update copyright info (scripts/copyright_notebooks.py) can be called via the Makefile (make verify-copyrightandmake copyrightrespectively).READMEs have been updated to accommodate these changes:
How to test
Pull down this PR:
gh pr checkout 467:1. Generate new notebooks
Via the CLI
i. Call
make notebookand follow the prompts. Note that the newly created notebook has the copyright info automatically added at the end.Important
Name the file
_notebook.ipynbso you can follow the instructions in "2. Update existing notebooks".ii. Delete the copyright cell from the notebook.
Using the end-to-end notebook
i. Open up
/notebooks/templates/e2e-notebook.ipynband run the notebook, following the prompts. Note that the newly created notebook has the copyright info automatically added at the end.Important
Name the file
notebook.ipynbso you can follow the instructions in "2. Update existing notebooks".ii. Delete the copyright cell from the notebook.
2. Update existing notebooks
Once you've deleted the existing copyright cell from the notebooks, call:
i. Run
make verify-copyright— You should get a return that one file (notebook.ipynb) is missing the copyright info. This is correct as the script only looks for standalone notebooks (ones that don't start with a_in front of the rest of the filename).ii. Run
make copyright— You should get a return that one file (notebook.ipynb) has been appended with the copyright info. 🎉What needs special review?
n/a
Dependencies, breaking changes, and deployment notes
documentation: docs notebooks: Updating copyright footer again documentation#1117demo-environment: https://github.com/validmind/demo-environment/pull/24Release notes
n/a
Checklist